Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @szadam)

-- commits line 2 at r1:
Provide proper author name.

docker/Dockerfile line 88 at r1 (raw file):

	openssl-libs \
	xz-libs \
	zlib \

I suppose these are redundant, please verify.

Code quote:

	daxctl-libs \
	kmod-libs \
	libgcc \
	libuuid \
	numactl-libs \
	openssl-libs \
	xz-libs \
	zlib \

docker/Dockerfile line 94 at r1 (raw file):

COPY hwloc.spec.mk /
COPY docker_install_hwloc.sh /
RUN /docker_install_hwloc.sh

Remove these files at the end of this dockerfile run.

Code quote:

COPY hwloc.spec.mk /
COPY docker_install_hwloc.sh /
RUN /docker_install_hwloc.sh

docker/docker_install_hwloc.sh line 5 at r1 (raw file):

# Copyright (C) 2020 - 2021 Intel Corporation.

# docker_install_hwloc.sh - is called inside a Docker container;

Comment not needed.

Code quote:

# docker_install_hwloc.sh - is called inside a Docker container;

docker/docker_install_hwloc.sh line 9 at r1 (raw file):

#
# Parameters:
# -memkind utils/docker directory

Misleading comment, no parameters are passed to this script.

Code quote:

# Parameters:
# -memkind utils/docker directory
#

docker/docker_install_hwloc.sh line 14 at r1 (raw file):

set -e

UTILS_DOCKER_PATH=$1

Remove this parameter and its usages from the script.

docker/docker_install_hwloc.sh line 43 at r1 (raw file):

#     rpmbuild -ba $SPEC
#     sudo rpm -i $RPMDIR/RPMS/$RPM_ARCH/*.rpm
# else

Remove commented out code.

Code quote:

# if [[ $(cat /etc/os-release) = *"fedora"* ]]; then
#     rpmdev-setuptree

#     SPEC="$UTILS_DOCKER_PATH"/hwloc.spec.mk
#     RPMDIR=$HOME/rpmbuild
#     RPM_ARCH=$(uname -m)

#     cp "$HWLOC_LOCAL_TAR_GZ" "$RPMDIR/SOURCES/${HWLOC_TAR_GZ}"

#     rpmbuild -ba $SPEC
#     sudo rpm -i $RPMDIR/RPMS/$RPM_ARCH/*.rpm
# else

Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @szadam)

docker/Dockerfile line 92 at r1 (raw file):

 && dnf clean all
 
COPY hwloc.spec.mk /

This file is not provided with this commit. It's not needed.

Reviewable status: 0 of 2 files reviewed, 8 unresolved discussions (waiting on @PatKamin)

-- commits line 2 at r1:

Done.

docker/Dockerfile line 88 at r1 (raw file):

you're right

docker/Dockerfile line 92 at r1 (raw file):

done

docker/Dockerfile line 94 at r1 (raw file):

Done.

docker/docker_install_hwloc.sh line 5 at r1 (raw file):

Done.

docker/docker_install_hwloc.sh line 9 at r1 (raw file):

Done.

docker/docker_install_hwloc.sh line 14 at r1 (raw file):

Done.

docker/docker_install_hwloc.sh line 43 at r1 (raw file):

Done.

Reviewed 2 of 2 files at r2.
Reviewable status: all files reviewed (commit messages unreviewed), 3 unresolved discussions (waiting on @szadam)

docker/Dockerfile line 85 at r2 (raw file):

 
COPY docker_install_hwloc.sh /
RUN /docker_install_hwloc.sh

nit: Move this just before memkind installation

Code quote:

COPY docker_install_hwloc.sh /
RUN /docker_install_hwloc.sh

docker/docker_install_hwloc.sh line 30 at r2 (raw file):

    make -j "$(nproc)"
    sudo make -j "$(nproc)" install
# fi

Remove this as well.

Code quote:

# fi

Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @PatKamin)

docker/docker_install_hwloc.sh line 30 at r2 (raw file):

Done.

Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @szadam)

docker/Dockerfile line 56 at r3 (raw file):

	ncurses-devel\
	ndctl-devel\
	numactl\

Is it needed? Please, verify.

docker/Dockerfile line 124 at r3 (raw file):

RUN /tz.sh

RUN rm /pmdk.sh /valgrind.sh /pmemobj-cpp.sh /pmemkv.sh /setup-maven-settings.sh /pmemkv-java.sh /pmemkv-python.sh /pmemkv-nodejs.sh /pmemkv-ruby.sh /memkind.sh /librpma.sh /tz.sh /docker_install_hwloc.sh

nit: Please, preserve the order of removed scripts to align with the order of executed scripts for better readability.

Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion (waiting on @PatKamin)

docker/Dockerfile line 56 at r3 (raw file):

You're right. It isn't needed.

docker/Dockerfile line 124 at r3 (raw file):

done

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @szadam)

Reviewed 1 of 2 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @szadam)

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @szadam)

Reviewed all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @szadam)

a discussion (no related file):
and just to be sure... have you tested it? will your memkind example work with that? 😉

docker/Dockerfile line 112 at r4 (raw file):

COPY docker_install_hwloc.sh /
RUN /docker_install_hwloc.sh

pls rename to be consistent with other script names, e.g., install-hwloc.sh

docker/docker_install_hwloc.sh line 3 at r4 (raw file):

#!/bin/bash
# SPDX-License-Identifier: BSD-2-Clause
# Copyright (C) 2020 - 2021 Intel Corporation.

-2022

docker/docker_install_hwloc.sh line 13 at r4 (raw file):

HWLOC_TAR_GZ=hwloc-"${HWLOC_VERSION}".tar.gz

HWLOC_TARBALL_URL=https://download.open-mpi.org/release/hwloc/"$HWLOC_LIBRARY_VERSION"/"$HWLOC_TAR_GZ"

above, you allow for the HWLOC_LIBRARY_VERSION to be unset (hence the default "1"), but here you expect it to be set...

did you mean to use HWLOC_VERSION instead?

docker/docker_install_hwloc.sh line 15 at r4 (raw file):

HWLOC_TARBALL_URL=https://download.open-mpi.org/release/hwloc/"$HWLOC_LIBRARY_VERSION"/"$HWLOC_TAR_GZ"

HWLOC_LOCAL_DIR="$HOME"/hwloc/"$HWLOC_LIBRARY_VERSION"

do you need these paths to be so complicated, why not just use /tmp for downloading this tarball?

docker/docker_install_hwloc.sh line 25 at r4 (raw file):

tar -xzf "$HWLOC_LOCAL_TAR_GZ" -C "$HWLOC_LOCAL_DIR" --strip-components=1

    # go to hwloc directory, build and install library

why is this indented?

docker/docker_install_hwloc.sh line 26 at r4 (raw file):

    # go to hwloc directory, build and install library
    cd "$HWLOC_LOCAL_DIR"

pls use pushd <dir> command (instead of cd)

and then, when done add a line with cd back = popd

docker/docker_install_hwloc.sh line 29 at r4 (raw file):

    ./configure --prefix=/usr
    make -j "$(nproc)"
    sudo make -j "$(nproc)" install

sudo is redundant...?

Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @lukaszstolarczuk)

a discussion (no related file):

yes ;)

docker/docker_install_hwloc.sh line 13 at r4 (raw file):

This is Rafał Rudnicki example. I just checked that it works and added a small correction

Reviewed 2 of 2 files at r5, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @lukaszstolarczuk)

docker/Dockerfile line 112 at r4 (raw file):

Done.

docker/docker_install_hwloc.sh line 3 at r4 (raw file):

Done.

docker/docker_install_hwloc.sh line 13 at r4 (raw file):

It is working fine.

docker/docker_install_hwloc.sh line 15 at r4 (raw file):

It is working fine.

docker/docker_install_hwloc.sh line 25 at r4 (raw file):

Done.

docker/docker_install_hwloc.sh line 26 at r4 (raw file):

It is working fine.

docker/docker_install_hwloc.sh line 29 at r4 (raw file):

yes

I guess, since it works, we shouldn't change much, at the moment

Reviewed 2 of 2 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @szadam)